Migrate functionName to just pipe namedFunction into name#1518
Closed
Migrate functionName to just pipe namedFunction into name#1518
functionName to just pipe namedFunction into name#1518Conversation
5 tasks
pokey
added a commit
that referenced
this pull request
Jun 16, 2023
- Fixes #711 - Fixes #1365 - Fixes #1238 ## Checklist - [x] Make sure that `"name"` behaves as well as `"funk name"` now does - [x] Create PR with migration `"funk name"` => `"name funk"`, and then remove all the `"funk name"` stuff (#1518). Might want to bail on that - [x] I have added [tests](https://www.cursorless.org/docs/contributing/test-case-recorder/) - [ ] I have updated the [docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and [cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet) - [ ] I have not broken the cheatsheet
pokey
commented
Jun 22, 2023
| return [modifier]; | ||
| } | ||
|
|
||
| return [ |
Member
Author
There was a problem hiding this comment.
show error message here, but still do the migration
Member
Author
|
After discussing with users, we've decided that we can't get rid of For |
Member
|
Replaced by #3038 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I had the thought that with our compositional pipelines,
"funk name"and"class name"should just be equivalent to"name funk"and"name class", respectively, as the latter would just expand to function and then get its name. This would simplify implementation, and lead to more consistency.I implemented the migration, and 10 tests fail.
On the bright side, most of the failures reveal inconsistencies in the way we handle
"funk"vs"name"vs"funk name", ie"funk name"sees something as a function but"funk"misses it, and should be fixed, or"name"is missing something that"funk name"hits, and should be fixedUnfortunately, there are a couple of test cases that are more problematic. The biggest problem is the C++ test
cursorless/packages/cursorless-vscode-e2e/src/suite/fixtures/recorded/languages/cpp/takeClassName3.yml
Line 10 in 65fab51
"class name"selectsClassNamebefore the::, but of course"name class"breaks because we're not in a class. Not sure what to do about this oneChecklist
functionNameandclassNamefromcursorless/packages/common/src/types/command/PartialTargetDescriptor.types.ts
Lines 75 to 133 in 65fab51
TODOcomment, and remaining more complex modifiers that contain other modifiers